feat: rewrite rounding as a string-digit algorithm; support all RoundingMode modes and bcdivmod (closes #36, #39)#82
Conversation
symfony/polyfill-php84 との差異調査 (#81) を踏まえ、丸め経路を phpseclib BigInteger 依存から純粋な文字列桁演算へ刷新する。 - RoundingMode を backed string enum から純粋 enum に変更し、 ネイティブ PHP 8.4 と完全一致させる (fix 1) - bcround の HalfEven/HalfOdd における float フォールバックを撤廃。 大きい数・高精度でもネイティブと一致する任意精度丸めに (closes #36) - 方向系4モード (TowardsZero/AwayFromZero/PositiveInfinity/ NegativeInfinity) を実装し、負 precision でもネイティブ一致 - bcfloor/bcceil を丸めコアへ委譲し高速化 (bcceil の add 依存を解消) - bcdivmod を BCMath::div/mod ベースで実装。拡張なし環境でも動作する 真のポリフィルとなる (closes #39) - bc関数ラッパーの引数名を $operand から $num へ統一しネイティブ準拠。 名前付き引数に対応 (fix 2) 丸めアルゴリズムは symfony/polyfill-php84 (MIT) の手法を参考にした clean re-implementation で、コードの複製はしていない。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- 方向系モードが ValueError を投げる前提だった旧テストを、 ネイティブ一致の正結果検証へ変更 - 高精度 HalfEven/HalfOdd の回帰テストを追加 (float フォールバック 時代のバグ: 12345678901234567890.5 や 2^53 付近の欠落を検出) - bcdivmod / 0除算のテストを追加 - phpstan: 純粋 enum 化に伴う match の default 追加と、bcmath 拡張 ロード時にネイティブ bcround 型で解決される場合の ignore を調整 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
方向系モードと bcdivmod の実装により、以下8件の公式 phpt テストが 拡張なし環境でも通るようになったため --skip から除外する: bcdivmod, bcdivmod_by_zero, bcround_all, bcround_away_from_zero, bcround_ceiling, bcround_floor, bcround_toward_zero, bcround_early_return README には bcdivmod・全RoundingMode対応と、symfony/polyfill-php84 との共存挙動 (PHP 8.2/8.3 + 拡張ありでのオートロード順) を追記。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughRoundingModeのpure enum化と方向系モード（TowardsZero、AwayFromZero、NegativeInfinity）対応を追加し、BCMathの丸めコアを文字列桁演算ベースへ再実装した。あわせてbcdivmod関数/BCMath::divmodメソッドを新規追加し、関連するテスト、README、CI、phpstan設定を更新した。 ChangesRoundingMode拡張とbcdivmod追加
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant BCMathRound as BCMath::round
participant ConvertMode as convertRoundingMode
participant RoundDigits as roundDigits
Caller->>BCMathRound: round(num, precision, mode)
BCMathRound->>ConvertMode: convertRoundingMode(mode)
ConvertMode-->>BCMathRound: ROUND_*トークン
BCMathRound->>RoundDigits: roundDigits(num, precision, token)
RoundDigits-->>BCMathRound: 丸め結果文字列
BCMathRound-->>Caller: 丸め結果
sequenceDiagram
participant Caller
participant bcdivmod
participant BCMathDivmod as BCMath::divmod
participant Div as div
participant Mod as mod
Caller->>bcdivmod: bcdivmod(num1, num2, scale)
bcdivmod->>BCMathDivmod: divmod(num1, num2, scale)
BCMathDivmod->>Div: div(num1, num2, 0)
BCMathDivmod->>Mod: mod(num1, num2, scale)
BCMathDivmod-->>bcdivmod: [商, 剰余]
bcdivmod-->>Caller: [商, 剰余]
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🚀 Performance Benchmark ResultsConfiguration
Performance SummaryPolyfill is 1.7x compared to native BCMath 📊 View Detailed ResultsBCMath Performance Benchmark Results
Note: Showing partial results. Run with Quick Summary🤖 Triggered by @nanasess with |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the PHP 8.4 bcmath polyfill by implementing the bcdivmod() function and fully supporting all eight native RoundingMode enum cases (including directional modes) using exact string-digit arithmetic. It also refactors the RoundingMode polyfill to be a pure enum, matching PHP 8.4's native behavior. The review feedback suggests a performance optimization for BCMath::divmod() to compute both the quotient and remainder in a single phpseclib BigInteger::divide() call, avoiding the overhead of performing the division twice through separate div() and mod() calls.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/BCMathTest.php (1)
2110-2132: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
testDivmodにネイティブbcdivmod()との比較を追加することを推奨。
testDiv/testMod/testRoundAllModesなど既存のテストはfunction_exists()でネイティブ関数が利用可能な場合にポリフィルとネイティブの結果を突き合わせていますが、testDivmodにはこのパターンがありません。CIのPHPTスキップリストからbcdivmod関連ケースが除去された（PR概要より）ことも踏まえると、PHP 8.4+環境でネイティブbcdivmod()とも一致することを検証しておくと、既存パターンとの一貫性が保て、リグレッション検出力も上がります。♻️ 提案する追加検証
foreach ($cases as [$num1, $num2, $scale, $expectedQuot, $expectedRem]) { [$quot, $rem] = BCMath::divmod($num1, $num2, $scale); $this->assertSame($expectedQuot, $quot, "quotient of {$num1} / {$num2} @ {$scale}"); $this->assertSame($expectedRem, $rem, "remainder of {$num1} / {$num2} @ {$scale}"); + + if (function_exists('bcdivmod')) { + // `@phpstan-ignore-next-line` + $this->assertSame([$expectedQuot, $expectedRem], bcdivmod($num1, $num2, $scale)); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/BCMathTest.php` around lines 2110 - 2132, `testDivmod` is missing the same native-vs-polyfill consistency check used by `testDiv`, `testMod`, and `testRoundAllModes`. Update `BCMathTest::testDivmod` to conditionally compare `BCMath::divmod()` against native `bcdivmod()` when `function_exists('bcdivmod')` is true, using the same cases already in the test. Keep the existing quotient/remainder assertions, and add a native comparison path to verify both results match `bcdivmod()` in PHP 8.4+ environments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@phpstan.neon.dist`:
- Line 32: The PHPStan ignore pattern in the config is treating `string|int` as
a regex alternation instead of a literal union type, so the match is too broad.
Update the message pattern in the phpstan.neon.dist entry to escape the pipe in
`(string|int) given` so it matches the literal text emitted by
`BCMath::round`/`bcround` and not regex alternatives.
In `@src/BCMath.php`:
- Around line 1158-1160: In BCMath’s rounding/shift handling, the half-rounding
path still builds a huge zero-filled string via str_repeat() when $precision is
very negative and the result should be 0. Update the relevant logic in BCMath so
that the zero-result case is detected and returned early before constructing
$scaledFrac, using the existing shifted/rounded-flow symbols around the $shift >
$intLength branch and the half-mode handling near the later rounding block.
- Around line 1045-1054: bcroundHelper() is bypassing the same validation path
used by round() because it calls roundDigits() directly. Update bcroundHelper()
so it delegates through round() or a shared validation helper, ensuring number,
precision, and mode are validated consistently before rounding. Keep the
existing bcroundHelper() and roundDigits() symbols in place, but make the public
helper follow the same checks as round() instead of returning or processing
invalid inputs directly.
---
Nitpick comments:
In `@tests/BCMathTest.php`:
- Around line 2110-2132: `testDivmod` is missing the same native-vs-polyfill
consistency check used by `testDiv`, `testMod`, and `testRoundAllModes`. Update
`BCMathTest::testDivmod` to conditionally compare `BCMath::divmod()` against
native `bcdivmod()` when `function_exists('bcdivmod')` is true, using the same
cases already in the test. Keep the existing quotient/remainder assertions, and
add a native comparison path to verify both results match `bcdivmod()` in PHP
8.4+ environments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22394793-5c45-4c68-b871-7c8a4e253c16
📒 Files selected for processing (7)
.github/workflows/ci.ymlREADME.mdlib/RoundingMode.phplib/bcmath.phpphpstan.neon.distsrc/BCMath.phptests/BCMathTest.php
- Key Differences 表を3者比較 (phpseclib/bcmath_compat, symfony/ polyfill-php84, bcmath-polyfill) に拡張。拡張非依存性・bcdivmod・ 全RoundingMode対応・純粋enum・丸め方式の違いを明記 - 古くなった「RoundingMode Enum Limitations」節を削除 (全モード対応済み) - PHP 8.1 の phpstan が in_array を常に true と誤判定する既存エラー (main でも failure) を ignore。bcpow を2引数目テストから除外する 意図的ガードのため該当コードは維持 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
nanasess
changed the title
- bcdivmod: 除算を1回に統合し高速化。剰余は num1 - num2×商 で明示計算し bcmod の切り捨て規約に一致させる(phpseclib divide() の剰余は非負規約で 負のオペランドでずれるため)(gemini 指摘) - bcroundHelper: round() へ委譲し、数値・precision・mode 検証を共通化 (coderabbit 指摘: 公開helperが検証を迂回していた) - 丸め: 巨大な負 precision でゼロ結果になる入力の str_repeat 割り当てを回避 (dropped桁は圧縮表現、ゼロ結果は短絡)(coderabbit 指摘) - testDivmod: ネイティブ bcdivmod との一致検証を追加(coderabbit 指摘) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
/benchmark |
🚀 Performance Benchmark ResultsBenchmark executed on merge to Configuration
Results📊 Detailed Benchmark Results (click to expand)BCMath Performance Benchmark Results
Summary🤖 Generated automatically by GitHub Actions |
1 participant
Overview
Following the source-level comparison with
symfony/polyfill-php84(#81), this PR rewrites the rounding path from a phpseclibBigInteger-based implementation to pure string-digit arithmetic and fills the remaining feature gaps. It combines the previously-planned PRs A/B/C into one, using a clean re-implementation (no code copied).RoundingModea pure enum (matching native PHP 8.4)$operand→$num(enables named arguments)TowardsZero,AwayFromZero,PositiveInfinity,NegativeInfinity)bcdivmod()on top ofBCMath::div/mod(works without the native extension)bcdivmod, all RoundingMode modes, and interoperability withsymfony/polyfill-php84Why rewrite the rounding
The previous
bcround()fell back toround((float) ...)forHalfEven/HalfOdd, abandoning arbitrary precision and returning wrong results for large or high-precision inputs (see #81):symfony processes every mode with pure string arithmetic (its rounding does not call the native bc extension), so adopting that approach improves correctness and performance at the same time.
Changes
lib/RoundingMode.phplib/bcmath.php$operand→$num; addbcdivmodwrappersrc/BCMath.phpconvertRoundingMode; routefloor/ceilthrough the core; adddivmod()tests/BCMathTest.phpbcdivmodtests.github/workflows/ci.yml--skipphpstan.neon.distin_arrayfalse positiveREADME.mdsymfony/polyfill-php84; note the coexistence behaviorVerification (local, PHP 8.5.4 with the bcmath extension)
PHPUnit: 279 tests, 1310 assertions, OK (added directional / high-precision / bcdivmod tests)
PHPStan (level max): No errors
php-cs-fixer: 0 findings
Native parity:
bcroundchecked over 8 modes × 21 inputs (including high precision and negative precision) → 0 mismatches;bcdivmodmatches too, and division by zero throwsDivisionByZeroError('Division by zero')Performance (N=20000, ms): dropping the BigInteger dependency speeds up the rounding functions
bcround(HalfAway)bcround30-digit high precisionbcceilPHPT skips resolved (no-extension environment)
The following 8 official phpt tests, previously skipped in
docker-phpt-tests, now pass and were removed from--skip:bcdivmod, bcdivmod_by_zero, bcround_all, bcround_away_from_zero, bcround_ceiling, bcround_floor, bcround_toward_zero, bcround_early_return(The remaining
scale_ini/gh20006/bcround_precision_boundsskips depend on the INI setting or theBcMath\NumberOOP API and are out of scope.)Compatibility / impact
ValueError, and the version-dependentis_numericbehavior are preserved).HalfAwayFromZero) and single-argumentfloor/ceilresults are unchanged (no impact on EC-CUBE's usage pattern; verified in Compatibility differences with symfony/polyfill-php84 when both are installed #81).Note on the PHPStan job
The
PHPStanjob was already failing onmainwith a single PHP 8.1-only false positive (in_array()reported as always-true attests/BCMathTest.php). It is unrelated to this PR; it is silenced here so the job can go green.Refs #81
Closes #36
Closes #39
🤖 Generated with Claude Code